FIX: inject_adapter incorrectly propagates inference_mode to active adapters#3290
Open
kiritozc wants to merge 2 commits into
Open
FIX: inject_adapter incorrectly propagates inference_mode to active adapters#3290kiritozc wants to merge 2 commits into
kiritozc wants to merge 2 commits into
Conversation
…ctive adapters When injecting a new adapter via inject_adapter, the housekeeping section called set_adapter(self.active_adapters, inference_mode=peft_config.inference_mode). Here peft_config belongs to the newly injected adapter, but self.active_adapters points to the existing active adapter(s). When the new adapter has inference_mode=True (e.g. during save_pretrained with path_initial_model_for_weight_conversion in PiSSA/OLoRA/CorDA workflows), this erroneously freezes the already-active training adapter, causing grad_norm to become 0 and training to effectively stop. The fix only propagates inference_mode when the new adapter IS the active adapter (first-time injection). For subsequent adapters, set_adapter is called without inference_mode, preserving the existing active adapter's trainability state. The new adapter's own inference_mode is still correctly handled by the existing code that follows. This was a regression introduced in commit 13fa0ae (PR huggingface#2765). A regression test is added that verifies adding an adapter with inference_mode=True does not freeze the existing active adapter.
BenjaminBossan
requested changes
Jun 1, 2026
BenjaminBossan
left a comment
BenjaminBossan
left a comment
Member
There was a problem hiding this comment.
Thanks for fixing this edge case. This fix looks good but I have two small comments regarding the test, please check.
| params_with_grad = [n for n, p in model.named_parameters() if p.requires_grad] | ||
| assert all(not p.requires_grad for p in model.parameters()) | ||
|
|
||
| @pytest.mark.parametrize("config_cls", [LoraConfig, LoHaConfig, LoKrConfig, IA3Config, OFTConfig, BOFTConfig]) |
Member
There was a problem hiding this comment.
OFT is failing to initialize with the given arguments. But I think for this test, just checking LoRA is enough (like in the previous tests).
| # Regression test for a bug where adding a second adapter with inference_mode=True would incorrectly freeze | ||
| # the already-active training adapter. This happened because inject_adapter propagated the new adapter's | ||
| # inference_mode to set_adapter for the existing active adapters. | ||
| # See PR #XXXX |
Author
There was a problem hiding this comment.
Hi, I've addressed both review comments:
- Simplified the test to only use LoraConfig
- Updated the PR reference in the comment (FIX: inject_adapter incorrectly propagates inference_mode to active adapters #3290)
Please take another look. Thanks!
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
BenjaminBossan
left a comment
BenjaminBossan
left a comment
Member
There was a problem hiding this comment.
A couple of tests are failing now. I think this is because they baked in the previous assumption and thus may require updating. Could you please take a look?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When injecting a new adapter via
inject_adapter, the housekeeping section calls:Here
peft_configbelongs to the newly injected adapter, butself.active_adapterspoints to the existing active adapter(s). When the new adapter hasinference_mode=True(e.g. duringsave_pretrainedwithpath_initial_model_for_weight_conversionin PiSSA/OLoRA/CorDA workflows), this erroneously freezes the already-active training adapter, causinggrad_normto become 0 and training to effectively stop.Fix
Only propagate
inference_modewhen the new adapter IS the active adapter (first-time injection). For subsequent adapters,set_adapteris called withoutinference_mode, preserving the existing active adapter's trainability state. The new adapter's owninference_modeis still correctly handled by the existing code below.Tests
test_inject_adapter_inference_mode_does_not_freeze_active_adapter— a regression test covering LoraConfig, LoHaConfig, LoKrConfig, IA3Config, OFTConfig, BOFTConfigswitch_inference_modeandadd_adapter) remain as xfail since they require decoupling active adapter selection fromrequires_gradinset_adapterRelated
13fa0aea(PR FIX: Wrong coupling between requires_grad and the active adapter #2765)